sockstat: Add automatic column sizing and remove -w option #1720

daminrido139 · 2025-06-12T03:49:09Z

Refactor sockstat to dynamically size table columns based on content.
This eliminates the need for the -w option, which is now ignored
for backwards compatibility.

Unknown fields are now consistently shown as "??" instead of a mix
of "", "?" and "?" for output uniformity.

Sponsored by: Google, LLC (GSoC 2025)
MFC after: 2 weeks

github-actions · 2025-06-12T03:49:19Z

Thank you for taking the time to contribute to FreeBSD!
There is an issue that needs to be fixed:

Missing Signed-off-by lines^c0df8c6

asomers · 2025-06-16T01:49:13Z

The automatic column sizing works pretty well. But it isn't perfect. On my machine, with almost any options, sockets whose protocol is "stream (not connected)" will have misaligned columns. This is easiest to see with the "-l" option. It looks like the "(not connected)" part isn't contributing to the calculation of the width of the PROTO column. For example:

USER   COMMAND    PID   FD PROTO  LOCAL ADDRESS                FOREIGN ADDRESS
somers sshd-sessi 56573 7  stream (not connected)              ??                           ??             
root   sshd-sessi 56571 7  stream (not connected)              ??                           ??             
somers sshd-sessi 4084  7  stream (not connected)              ??                           ??             
root   sshd-sessi 4082  7  stream (not connected)              ??                           ??             
root   sshd       3974  7  tcp6   *:22                         *:*            
root   sshd       3974  8  tcp4   *:22                         *:*            
root   syslogd    2679  6  udp6   *:514                        *:*            
root   syslogd    2679  7  udp4   *:514                        *:*            
root   syslogd    2679  8  dgram  /var/run/log                  <-            
root   syslogd    2679  10 dgram  /var/run/logpriv             ??             
root   nfsuserd   2080  3  udp6   *:633                        *:*            
root   nfsuserd   2079  3  udp6   *:633                        *:*            
root   nfsuserd   2078  3  udp6   *:633                        *:*            
root   nfsuserd   2077  3  udp6   *:633                        *:*            
root   nfsuserd   2076  3  udp6   *:633                        *:*            
root   devd       1593  4  stream /var/run/devd.pipe           ??             
root   devd       1593  5  seqpac /var/run/devd.seqpacket.pipe ??             
??     ??         ??    ?? stream (not connected)              ??                           ??

Also, there's another change that wasn't mentioned in the commit message. Is this deliberate?

Fields that were previously "" are now printed as "??".
Fields that were previously "?" for the USER, COMMAND, PID, and FD columns are now "??". Such sockets are most easily seen if NFS is in use.

daminrido139 · 2025-06-16T14:14:47Z

Thanks, @asomers,

You're absolutely right, "stream (not connected)" should be fully inside the PROTO column. I initially misunderstood and treated (not connected) as part of the LOCAL ADDRESS, but I’ll fix that and ensure the width calculation includes the full string.

Regarding the "??" change - yes, that was intentional. I wanted to make the output more uniform by using "??" for all unknown fields instead of mixing "" and "?". Sorry for not mentioning that in the commit message. happy to amend the commit if you prefer.

asomers · 2025-06-16T15:26:35Z

Thanks, @asomers,

You're absolutely right, "stream (not connected)" should be fully inside the PROTO column. I initially misunderstood and treated (not connected) as part of the LOCAL ADDRESS, but I’ll fix that and ensure the width calculation includes the full string.

Regarding the "??" change - yes, that was intentional. I wanted to make the output more uniform by using "??" for all unknown fields instead of mixing "" and "?". Sorry for not mentioning that in the commit message. happy to amend the commit if you prefer.

Using "??" uniformly is ok. The only risk is that it upsets some scripts that people use to parse sockstat's output with awk. But those scripts are fragile anyway. Using libxo would be much more reliable!

asomers · 2025-06-16T02:02:38Z

usr.bin/sockstat/sockstat.c

+				printf(" %-*s", cw->local_addr, "(not connected)");
+			strcpy(buf,"??");
+			if (laddr->address.ss_len > 0)
+				formataddr(&laddr->address, buf, sizeof(buf));


This line will overwrite the "??" from line 1399. If you meant for line 1399 to be more like a default initializer, I suggest you move it below, with else. There are similar patterns elsewhere, like on line 1407-1409.

asomers · 2025-06-16T19:47:36Z

usr.bin/sockstat/sockstat.c

@@ -1344,33 +1524,38 @@ display(void)
 	struct passwd *pwd;
 	struct file *xf;
 	struct sock *s;
-	int n, pos;
+	int n;
+    struct col_widths cw;


Some spaces snuck in on these two lines. You should consistently indent with tabs.

asomers · 2025-06-16T20:06:37Z

usr.bin/sockstat/sockstat.c

+static void
+calculate_column_widths(struct col_widths *cw)
+{
+    cw->user         	= 4;


Also here you indented with spaces. Please change them all to tabs. See style(9) for a detailed description of how to format C code.

fixed! Thanks for the reference.

asomers · 2025-06-16T20:54:03Z

usr.bin/sockstat/sockstat.c

+    cw->stack			= TCP_FUNCTION_NAME_LEN_MAX;
+    cw->cc				= TCP_CA_NAME_MAX;
+
+    char buf[512];


This is a lot of memory to put on the stack. Better to use malloc(). But in this case, you don't have to! It's possible to eliminate the need for buf entirely, but taking advantage of the fact that snprintf always returns the number of characters that it would've written if the buffer had been big enough. So just call len = snprintf(NULL, 0, ...) and remove all of the strlen lines.

Removed buf and strlen wherever possible by using len = snprintf(NULL, 0, ...) . Used malloc() instead of stack memory.

asomers · 2025-06-16T21:02:55Z

usr.bin/sockstat/sockstat.c

+	/* Remote peer we connect(2) to, if any. */
+	if (faddr->conn != 0) {
+		struct sock *p;
+		strcpy(buf, "-> ");


strcpy should almost always be avoided. Use strncpy, or better yet strlcpy.

Replaced strcpy with strlcpy.

Refactor sockstat to dynamically size table columns based on content. This eliminates the need for the -w option, which is now ignored for backwards compatibility. Unknown fields are now consistently shown as "??" instead of a mix of "", "?" and "?" for output uniformity. Sponsored by: Google, LLC (GSoC 2025) MFC after: 2 weeks

asomers

Pro tips:

Once a reviewer first looks at a PR, never force-push it. If you must amend
it, leave your changes in a separate commit. That makes it easy for the
reviewer to see what's changed since his first review.
Never merge the main branch into your PR branch. That can confuse the
history and doesn't really solve any problems. Instead, better to leave the
PR out-of-date unless it conflicts with the main branch. If you must resolve
conflicts, it's better to rebase than to merge.

I'll come back tomorrow for a lengthier review. I'll have to re-review the entire thing.

daminrido139 · 2025-06-24T03:52:06Z

Thanks for the advice, @asomers. I will make sure to follow this workflow and keep changes in separate commits after reviews begin.

asomers suggested changes Jun 16, 2025

View reviewed changes

daminrido139 force-pushed the gsoc-sockstat-libxo branch from 9761665 to 7c8ce27 Compare June 18, 2025 11:59

daminrido139 force-pushed the gsoc-sockstat-libxo branch from 7c8ce27 to c0df8c6 Compare June 20, 2025 13:45

asomers reviewed Jun 24, 2025

View reviewed changes

sockstat: Add automatic column sizing and remove -w option #1720

Are you sure you want to change the base?

sockstat: Add automatic column sizing and remove -w option #1720

Uh oh!

Conversation

daminrido139 commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asomers commented Jun 16, 2025

Uh oh!

daminrido139 commented Jun 16, 2025

Uh oh!

asomers commented Jun 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asomers left a comment

Choose a reason for hiding this comment

Uh oh!

daminrido139 commented Jun 24, 2025

Uh oh!

Uh oh!

daminrido139 commented Jun 12, 2025 •

edited

Loading

github-actions bot commented Jun 12, 2025 •

edited

Loading